Issue 480 - Add .map file extension support to dash #478
Conversation
Marc-Andre-Rivet
changed the title
|
@T4rk1n I've created an issue to track this PR in a more normal manner and linking it above in the description. Also updated the changelog. As it should have been done from the start :) |
dash/dash.py
Outdated
| is_dynamic_resource = ( | ||
| 'dynamic' in resource and | ||
| resource['dynamic'] is True | ||
| ) |
There was a problem hiding this comment.
Not sure what the preferred indentation pattern is for Python -- went with something that seemed natural with the syntax
There was a problem hiding this comment.
No () and single line, we prefer explicit: if 'dynamic' in resources and resource.get('dynamic'). Also .get over [key] to avoid unnecessary KeyErrors. Only use is for comparing pointer references, ie is None to assert null and not just falsy.
There was a problem hiding this comment.
Did this because the line is too long for flake8's taste.
There was a problem hiding this comment.
How do I one line this and make flake8 happy? Apart from an actual function, etc.
There was a problem hiding this comment.
Break them using \ at the end of the line.
There was a problem hiding this comment.
Getting the same error from pylint
There was a problem hiding this comment.
And now this.. dash/dash.py:355:4: R0912: Too many branches (13/12) (too-many-branches
I guess the additional if triggered this rule.
There was a problem hiding this comment.
I prefer if cond to is True, is True is when you need to assert that it the actual True and not another type that evaluate to True in a condition like a string.
There was a problem hiding this comment.
Ah! Misread! No need for the explicit comparison... all good haha
There was a problem hiding this comment.
Add # pylint: disable=too-many-branches to the start of the function.
dash/dash.py
Outdated
| )) | ||
| ) | ||
| if not is_dynamic_resource: | ||
| srcs.append(path) |
There was a problem hiding this comment.
Same here..
dash/dash.py
Outdated
| @@ -1028,6 +1041,7 @@ def _walk_assets_directory(self): | |||
|
|
|||
| full = os.path.join(current, f) | |||
|
|
|||
| print(f) | |||
There was a problem hiding this comment.
Oups! Will remove
| @@ -19,6 +19,8 @@ def _filter_resources(self, all_resources, dev_bundles=False): | |||
| filtered_resources = [] | |||
| for s in all_resources: | |||
| filtered_resource = {} | |||
| if 'dynamic' in s: | |||
| filtered_resource['dynamic'] = s['dynamic'] | |||
There was a problem hiding this comment.
dynamic property needs to survive the sanitation process -- adding it to the list
There was a problem hiding this comment.
I made a few code style suggestions -- nothing critical. Also I made a few comments on previous discussions.
I'm also curious what the motivation behind 'dynamic' is? Do the .map files change at run-time independently of the bundles? I always thought they were static content, just static content that shouldn't be added to the <script> tag of a Dash app. Although, I am not sure what a good short word for that is which can be used as a variable name.
dash/dash.py
Outdated
| @@ -378,15 +379,25 @@ def _relative_url_path(relative_package_path='', namespace=''): | |||
|
|
|||
| srcs = [] | |||
| for resource in resources: | |||
| is_dynamic_resource = 'dynamic' in resource and \ | |||
| resource.get('dynamic') | |||
|
|
|||
| if 'relative_package_path' in resource: | |||
| if isinstance(resource['relative_package_path'], str): | |||
There was a problem hiding this comment.
An alternate way might solve the too-many-branches 🐫
paths = resource['relative_package_path']
paths = [paths] if isinstance(paths, str) else paths
for rel_path in paths:
... the logic that is repeated twice ...
There was a problem hiding this comment.
Sure.. I'm in, if only because it makes me learn the language haha!
dash/dash.py
Outdated
| @@ -378,15 +379,25 @@ def _relative_url_path(relative_package_path='', namespace=''): | |||
|
|
|||
| srcs = [] | |||
| for resource in resources: | |||
| is_dynamic_resource = 'dynamic' in resource and \ | |||
There was a problem hiding this comment.
perhaps resource.get('dynamic', False) so it is not split
There was a problem hiding this comment.
dictionary entry with default I guess?
dash/dash.py
Outdated
| )) | ||
| else: | ||
| for rel_path in resource['relative_package_path']: | ||
| self.registered_paths[resource['namespace']]\ |
There was a problem hiding this comment.
@rmarren1 Not sure this is considered correct (escaping with '' in the middle of a chain (as opposed to an 'if' as I've done elsewhere)
There was a problem hiding this comment.
Do you mean using the \? That is standard to do
| def _collect_and_register_resources(self, resources): | ||
| # now needs the app context. | ||
| # template in the necessary component suite JS bundles | ||
| # add the version number of the package as a query parameter | ||
| # for cache busting | ||
| def _relative_url_path(relative_package_path='', namespace=''): | ||
|
|
||
| # track the registered packages | ||
| self.registered_paths[namespace].add(relative_package_path) | ||
|
|
||
| module_path = os.path.join( |
There was a problem hiding this comment.
As suggested, moved the side-effect outside of the path calculator and calling it directly below
Good question. This is 'dynamic' in the same sense as Babel and Webpack defines it -- these files are loaded on a "per need" basis, or dynamically -- not to be confused with the other meaning of dynamic where the files would change at runtime. In that sense 'dynamic' really is the correct word vs. static, as opposed to say 'async'. This is useful for source maps but also when splitting code into chunks that are only needed if certain features / behaviors are activated. Imagine a hypothetical component that could connect to provider A or B based on some settings -- if these libraries are big, it makes sense to delay the loading of these dependencies to when (and if!) they are needed. This need is already foreseen in the dash-table where certain features will require us to load libraries that are bigger than the table's bundle itself :) Or the more mundane case where you would have images, icon libraries or fonts bigger than a certain threshold not be included in the main chunk but rather stand beside it. That being said -- I see your point that this could be confusing to users but I don't have a good alternative either. ... 'load_dynamically' or 'dynamic_load' maybe? |
|
Okay, thanks for the detailed explanation. I didn't know that terminology, but it makes sense to me now: 'dynamic' meaning roughly 'this resource will be dynamically loaded from the front-end when it is needed, so it is not included in the initial <script> tag load' Thanks for making those changes, looks a lot cleaner now! 💃 |
|
Since this evolved from being a small modification (.map support) to adding an entirely new feature (dynamic support) I think we should make this a minor version bump instead of a patch. |
|
^ Agreed |
- update changelog with dynamic
Fixes #480.
Adding support for .js.map files in Dash so source map file is accessible from browser when available
Also, adding basic support for dynamic file loading. Otherwise the map gets injected as a script in the page, killing perf and just being generally useless!